-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/gv html refactor #136
Conversation
This is not meant to be the definitive refactor, since there will be a lot of changes introduced in v0.3 onward, but it cleans up quite a bit and should be enough for now. |
fd50998
to
cc05ef0
Compare
Good calls, all addressed in the latest commit. Thanks! |
e7ac4f7
to
21e514b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggest to remove line breaks from manufacturer info written to the BOM. I was not able to comment directly on the lines you didn't change now, but here is the full diff:
---------------------------- src/wireviz/Harness.py ----------------------------
index 69e0443..4c93895 100644
@@ -197,8 +197,9 @@ class Harness:
wireidentification = []
if isinstance(cable.pn, list):
wireidentification.append(f'P/N: {cable.pn[i - 1]}')
- manufacturer_info = manufacturer_info_field(cable.manufacturer[i - 1] if isinstance(cable.manufacturer, list) else None,
- cable.mpn[i - 1] if isinstance(cable.mpn, list) else None)
+ manufacturer_info = manufacturer_info_field(
+ cable.manufacturer[i - 1] if isinstance(cable.manufacturer, list) else None,
+ cable.mpn[i - 1] if isinstance(cable.mpn, list) else None)
if manufacturer_info:
wireidentification.append(html_line_breaks(manufacturer_info))
# print parameters into a table row under the wire
@@ -335,7 +336,7 @@ class Harness:
conn_color = f', {shared.color}' if shared.color else ''
name = f'Connector{conn_type}{conn_subtype}{conn_pincount}{conn_color}'
item = {'item': name, 'qty': len(designators), 'unit': '', 'designators': designators if shared.show_name else '',
- 'manufacturer': shared.manufacturer, 'mpn': shared.mpn, 'pn': shared.pn}
+ 'manufacturer': remove_line_breaks(shared.manufacturer), 'mpn': remove_line_breaks(shared.mpn), 'pn': shared.pn}
bom_connectors.append(item)
bom_connectors = sorted(bom_connectors, key=lambda k: k['item']) # https://stackoverflow.com/a/73050
bom.extend(bom_connectors)
@@ -354,7 +355,7 @@ class Harness:
shield_name = ' shielded' if shared.shield else ''
name = f'Cable{cable_type}, {shared.wirecount}{gauge_name}{shield_name}'
item = {'item': name, 'qty': round(total_length, 3), 'unit': 'm', 'designators': designators,
- 'manufacturer': shared.manufacturer, 'mpn': shared.mpn, 'pn': shared.pn}
+ 'manufacturer': remove_line_breaks(shared.manufacturer), 'mpn': remove_line_breaks(shared.mpn), 'pn': shared.pn}
bom_cables.append(item)
# bundles (ignores wirecount)
wirelist = []
@@ -364,8 +365,8 @@ class Harness:
# add each wire from each bundle to the wirelist
for index, color in enumerate(bundle.colors, 0):
wirelist.append({'type': bundle.type, 'gauge': bundle.gauge, 'gauge_unit': bundle.gauge_unit, 'length': bundle.length, 'color': color, 'designator': bundle.name,
- 'manufacturer': index_if_list(bundle.manufacturer, index),
- 'mpn': index_if_list(bundle.mpn, index),
+ 'manufacturer': remove_line_breaks(index_if_list(bundle.manufacturer, index)),
+ 'mpn': remove_line_breaks(index_if_list(bundle.mpn, index)),
'pn': index_if_list(bundle.pn, index)})
# join similar wires from all the bundles to a single BOM item
wire_group = lambda w: (w.get('type', None), w['gauge'], w['gauge_unit'], w['color'], w['manufacturer'], w['mpn'], w['pn'])
Will include both your suggestions. Maybe the new CSV library coming in #123 will handle line breaks in a better way, too. One more thing I am thinking about is replacing all html = f'{html}<some code>` with html = []
html.append('<some code>')
...
html='\n'.join(html) which would make running html = f'{html}<some code>\n` for every line. Will play with this when I have more time. |
Thank's.
Testing is needed, I guess...
I really like your idea! 👍 You are also getting closer to something similar to |
Implementation is complete, I'm quite pleased with the result (especially the much more readable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I really like this new output with the HTML split into many
'\n'
separated rows of code that simplify code inspection during development and reduce the diffs! 😃 I would have preferred moving thelabel
attributes containing HTML to the end of each node declaration to avoid "hiding" the remaining node attributes located after all HTML lines, but unfortunately, it seems thedot.node()
function have hardcoded thelabel
to be first attribute. -
I'm not quite sure if I have done this review correctly. 😟 Some of the code fragments I have commented below, are marked as Outdated, and I don't know why.
-
Several of my comments are just different instances of the same issue. Each issue is explained at least once, and the other issue instances might have a simplified comment only, but my comments below are unfortunately not shown in the same order as when I wrote the comments, and therefore the explaining comment might not be the first one in all cases.
-
You might not share my opinion about all these issues, and I respect that. These two issues, in particular, I'm not completely convinced about (maybe it's better to just keep your code version in these cases?):
- Replacing consecutive
append()
calls with a singleextend()
call? (Mixingappend()
andextend()
a lot might be bad for consistency and readability.) - Avoid reusing the variable when converting to another type? (Maybe OK as long as there is no doubt about what it contains.)
- Replacing consecutive
-
When there is a good reason for appending to a list that is just initialized to an empty list, then I also suggest adding a small comment that explains this reason, e.g. something like this:
html = [] # Using append() to assign the first row below for consistency with the other rows
html.append('<table...>')
-
I have used the term
row
in several of my suggestions (e.g. above) where I talk about'\n'
separated rows of HTML code. Do you think this term might be misunderstood as HTML table rows? -
I'm sorry for creating all the noise with a high number of comments about repeated issues that I'm not fully convinced about. 😢 However, there are also a few other minor issues, and I hope they don't drown in the pool of repeated issues.
-
I wonder why you have chosen to first generate an outer table containing some placeholder comments, then generate a new inner table, and then replacing any placeholder in the outer table with the inner table? Isn't it possible (and more readable code) to first building the inner table, and then inserting that directly (instead of a placeholder) when building the outer table? When you need to replace some part of the tag that
nested_html_table()
inserts just before the placeholder, then I understand areplace()
must be done afterwords, but that's only needed for the<!-- colorbar -->
placeholders. It's not a major issue - I'm just curious.
@@ -97,33 +99,36 @@ def create_graph(self) -> Graph: | |||
connector.color, '<!-- colorbar -->' if connector.color else None], | |||
'<!-- connector table -->' if connector.style != 'simple' else None, | |||
[html_line_breaks(connector.notes)]] | |||
html = nested_html_table(rows) | |||
html.extend(nested_html_table(rows)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider html = nested_html_table(rows)
instead, or is there a good reason behind extending an empty list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I find it more understandable to start with an empty list (line 91) and then populate it slowly... a matter of style, nothing else.
src/wireviz/Harness.py
Outdated
for pin, pinlabel in zip(connector.pins, connector.pinlabels): | ||
if connector.hide_disconnected_pins and not connector.visible_pins.get(pin, False): | ||
continue | ||
pinlist.append([f'<td port="p{pin}l">{pin}</td>' if connector.ports_left else None, | ||
f'<td>{pinlabel}</td>' if pinlabel else '', | ||
f'<td port="p{pin}r">{pin}</td>' if connector.ports_right else None]) | ||
|
||
pinhtml = '<table border="0" cellspacing="0" cellpadding="3" cellborder="1">' | ||
pinhtml.append('<table border="0" cellspacing="0" cellpadding="3" cellborder="1">') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider pinhtml = ['<table border="0" cellspacing="0" cellpadding="3" cellborder="1">']
instead, or is there a good reason behind appending to an empty list?
html = '\n'.join(html) | ||
dot.node(cable.name, label=f'<\n{html}\n>', shape='box', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider htmlstr = '\n'.join(html)
to avoid reusing the list variable as a string variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I will leave it as is, since the join
can't be built into the f-String easily... the variable is never used again after the following line, so I don't see an issue with reusing it.
src/wireviz/Harness.py
Outdated
pinhtml.append(f'{column}') | ||
pinhtml.append('</tr>') | ||
pinhtml.append('</table>') | ||
pinhtml = '\n'.join(pinhtml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider pinhtmlstr = '\n'.join(pinhtml)
to avoid reusing the same variable with another type. There are different opinions about this subject, though. Feel free to ignore my comments about this. There are several instances of this issue in the code, and I assume you either keep them all as is, or change them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's terrible to reuse the name in this instance, but I have come up with a compromise that doesn't require pinhtmlstr
either.
html = [row.replace('<!-- connector table -->', '\n'.join(pinhtml)) for row in html]
@formatc1702 When trying to rebase my PR #137 on top of this branch, I discovered an issue that I didn't see during my last review:
|
@kvid, I have built in some of your suggested changes. I've left the Regarding borders around bundle notes: It's a side-effect of refactoring but I would argue it is more consisten this way... Otherwise the notes just float around below the wires and their manufacturer info. |
That's perfectly fine with me. As I wrote in the main comment of #136 (review), I was not fully convinced about the single
I don't disagree with you. I just wanted to know it was intentional, and not something you would like to reverse very soon. 😃
I only have my phone here now, and are unable to test your new commit until I'm back to my PC probably later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reading my review and accepting a selection of my suggestions. I hope you also read my main review comment #136 (review), and not only the code fragment comments.
@formatc1702 wrote:
I don't think it's terrible to reuse the name in this instance, but I have come up with a compromise that doesn't require pinhtmlstr either.
html = [row.replace('<!-- connector table -->', '\n'.join(pinhtml)) for row in html]
As long as you are not afraid of a potential increase of running-time (I have not done measurements) when doing the join inside the loop for each row of the connector HTML and similar for the cable HTML, then go ahead merging.
The HTML output looks quite good. Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated? Where should such a description be stored and updated to improve consistency when new HTML entries are introduced in the future? Maybe close to the nested_html_table()
implementation?
Have you considered a single space indentation for each sub-level of HTML tag structure? The up-side is HTML readability, but the down-side is an increased diff when moving sections to a different sub-level.
Thanks for bringing the comment to my attention. I read it when you first posted it, but I must have missed some of the later edits. I will address some open points below; most of them have already been addressed elsewhere.
I'm not worried about runtime for WireViz at this moment, and there are probably bigger issues in this respect anyway. Comparing the performance on this issue is probably more of academit interest ;-)
No hard rules right now, and it's not terribly important. Cells are usually in one row (
About your comment
Annoying, yes. Oh well...
Not sure when you added this sentence... if I push new commits that address your issues, they become outdated since the commit they refer to is no longer the current one. At least that is my theory. So it's probably a good sign in this case.
I think the comment adds more noise than the extra
It's a bit of a development artifact, yes. Part of me agrees with you that inserting the content directly makes more sense. Part of me likes defining the broad structure first, and then filling in the details, and "hacking" the existing code to change the Please let me know if there are any other open points I should address! |
I agree. It was more like a joke. 😄
Nice. Should we but these words into a comment in the code as soft recommendations? Maybe into
It is not a major issue to me, especially if the Python code gets more complicated and less readable. I will wait and see what you suggest.
I see your point, and have changed my mind. I now agree with you.
See commit 9c933d1 in PR #137 where I changed the colorbar code to something similar to my newest image code. That PR is now based on this PR and using your new refactoring, by the way.
None that I can think of right now... |
A new module for the HTML stuff sounds good, as a separate issue/PR!
It was less painful than anticipated, and I do like the output! Have a look at the latest commit. This will hopefully be the last change before the merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation for this merge? I can't see any real change of code. What is then gained, besides more complex commit history?
You mean 97a9de4? I just discovered it too. I have absolutely no idea where it came from, or why it happened... especially since no code changes 😕
I was just referring to merging this feature into |
34907ed
to
4093a6a
Compare
Should be fixed now. Very strange. On a separate note: |
Thank's for the fix.
You can go ahead rebasing. My repo will still contain copies of your current commits, and I can rebase my branch on top of your updated branch afterwards. |
All good regarding the HTML whitespace feature then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The output look good, and don't think the Python code readability was harmed.
to match the one for connectors.
Improve code based on review Suggestions by @kvid
Simplify code remove superfluous temporary variables `pinlist`, `wirerow` Add suggested changes
4093a6a
to
e3fb39f
Compare
Fixes #66.
Adds a
color
property to cables, to match the behavior of connectors, for consistency.This might be useful, for example, for highlighting cables that remain live after an emergency stop or mains disconnection, that need to be orange-coloured according to certain standards (Source in German).